Skip to content

chore: remove occurrences of empty() #374

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 6, 2025

Conversation

alexandre-daubois
Copy link
Contributor

empty() is used less and less in favor of bool casts on arrays. Symfony, for example, banned it from its codebase. I suggest we remove them from here as well.

@DZunke
Copy link
Contributor

DZunke commented Jul 3, 2025

May i ask why those bool casts are favored instead of replacing empty by explicit checks? I understand the downsides of empty but it feels weird to check if an empty array is false instead of explicit check like \count($foo) === 0.

I just try to understand the pro and con of this variant 🤔

@alexandre-daubois
Copy link
Contributor Author

I'm in favor of removing empty(), but I'm also fine using \count($foo) === 0 to check array emptiness. The main difference is that by using a bool cast, this check is performed at the engine level instead of userland, which makes it more efficient. I can understand performance in not what matters the most here, and I can update the PR to use count() if you prefer. As long as empty() is avoided 😄

@chr-hertel
Copy link
Member

Aaah, good reminder, didn't catch that we're using it that often already 😬

I'm in favor of using explicit checks over implicit bool casts as well - at least for errors. And I'm afraid that with merging #369 I created some conflict here

@alexandre-daubois
Copy link
Contributor Author

Alright! Explicit checks added on arrays

Copy link
Member

@chr-hertel chr-hertel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only make cs and we're good to go - thanks @alexandre-daubois! :)

@alexandre-daubois
Copy link
Contributor Author

Ah yes of course, done 🙂

@chr-hertel chr-hertel merged commit 7cd36eb into php-llm:main Jul 6, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants